Skip to content

Fix order filter with embedded and nulls comparison#4151

Merged
alanpoulain merged 3 commits into
api-platform:2.6from
walva:fix-embeddable-filter
Mar 22, 2021
Merged

Fix order filter with embedded and nulls comparison#4151
alanpoulain merged 3 commits into
api-platform:2.6from
walva:fix-embeddable-filter

Conversation

@walva

@walva walva commented Mar 21, 2021

Copy link
Copy Markdown
Contributor
Q A
Branch? 2.6
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fixes #4144 Fixes #3440
License MIT
Doc PR Feature already documented here: Order Filter (Sorting) > Comparing with Null Values

Description

Comparing with Null Values doesn't work with embeddable.

Thanks @vsalvans who found the issue in #3440.

Todo

  • Always add tests and ensure they pass. (PhpUnit)
  • Always add tests and ensure they pass. (Behat)
  • Update CHANGELOG.md file
  • Add test for mondoDB ?

Help wanted

Issue n°1: Is there a better way to add a test for this bug ?

I wrote a lot of PhpUnits Tests code. First, I started to test the correct behavior of OrderFilter regarding embaddables. I modified \ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Dummy to add a public $embeddedDummy; property.
By doing this, I was able to add a new test scenario under \ApiPlatform\Core\Tests\Bridge\Doctrine\Common\Filter\OrderFilterTestTrait::provideApplyTestArguments :

'embeddable_nulls_comparison' => [
    [
        'embeddedDummy.dummyName' => [
            'nulls_comparison' => 'nulls_largest',
        ],
    ],
    [
        'order' => [
            'embeddedDummy.dummyName' => 'asc',
        ],
    ],
],

Unfortunatly, adding a new property created several new entries in the API documentation which led me writing a lot of code for all other tests (eg: BooleanFilterTest, RangeFilterTest, etc...). Regarding the impacts in the tests (see above), I am not convinced it is the best way to go. Should I create more isolated tests specific to embeddable somewhere else ?

Issue n°2: behat tests fails

While all PhpUnit tests succeed, the behats tests fail. The first failing scenario is creating a new Dummy resource using GraphQL. The reason is that to create a new Dummy entity, it now needs the missing part of public $embeddedDummy; in the payload.
This encourages me thinking this is not the best way to go.

001 Scenario: Create an item with a subresource                                     # features/graphql/mutation.feature:428
      And the JSON node "data.createDummy.dummy.id" should be equal to "/dummies/2" # features/graphql/mutation.feature:451
        Failed to evaluate expression 'data.createDummy.dummy.id' (Exception)

Issue n°3: I didn't write tests for mongoDB, but tests are green.

Am I missing something here ? Also, I never used mongoDB, and I'm not even sure the bug affect this database.

Comment thread src/Bridge/Doctrine/Orm/Filter/OrderFilter.php Outdated
Comment thread src/Bridge/Doctrine/Orm/Filter/OrderFilter.php
@alanpoulain

Copy link
Copy Markdown
Member

Should I create more isolated tests specific to embeddable somewhere else ?

Yes I think it would be the best course of action. We can add an optional parameter to use another resource to do the tests in DoctrineOrmFilterTestCase.

For MongoDB, there is no null comparison, you don't need to change anything.

@walva walva force-pushed the fix-embeddable-filter branch from b126d8a to 175afd2 Compare March 21, 2021 22:11
@walva

walva commented Mar 21, 2021

Copy link
Copy Markdown
Contributor Author

I force pushed another branch with a new approach based on previous feedback. WDYT ?
If the current version is better, what would be missing?

  • Update CHANGELOG.md file

@walva

walva commented Mar 21, 2021

Copy link
Copy Markdown
Contributor Author

Also, I couldn't download PHP CS Fixer tool, website seems to be down at the moment.
(tools mentionned here : https://github.com/api-platform/core/blob/main/CONTRIBUTING.md )

@alanpoulain alanpoulain changed the title WIP: Fix embeddable filter Fix order filter with embedded and nulls comparison Mar 22, 2021
@alanpoulain

Copy link
Copy Markdown
Member

I made some little fixes, I think it's good to merge 🙂

@alanpoulain alanpoulain merged commit 3930b86 into api-platform:2.6 Mar 22, 2021
@alanpoulain

Copy link
Copy Markdown
Member

Thank you @walva.

@walva

walva commented Mar 22, 2021

Copy link
Copy Markdown
Contributor Author

Thank you too! Very instructive

@vsalvans

Copy link
Copy Markdown

Thank you guys for all work done!!

@t-richard

Copy link
Copy Markdown

Nice to see it was fixed 🤓

Thanks @walva for the work done here 💪

@walva walva deleted the fix-embeddable-filter branch March 31, 2021 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants